Skip to content

Conversation

cgwalters
Copy link
Collaborator

No description provided.

@bootc-bot bootc-bot bot requested a review from jeckersb October 2, 2025 18:09
@github-actions github-actions bot added area/install Issues related to `bootc install` area/documentation Updates to the documentation labels Oct 2, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant amount of work related to composefs, including switching the fs-verity hashing algorithm from SHA256 to SHA512, adding a new workflow for building 'sealed' images with Unified Kernel Images (UKIs), and making the composefs backend a default feature. The changes are mostly consistent and well-structured, with good use of type aliases to improve code clarity.

However, I've found a few issues that need attention:

  • There's some dead and potentially buggy code in the new xtask for building sealed images.
  • A new Dockerfile contains a leftover command that should be removed.
  • The documentation for newly added CLI options and subcommands is incomplete.

Please see my detailed comments for suggestions on how to address these points.

Comment on lines +117 to +131
**--composefs-native**



**--insecure**



Default: false

**--bootloader**=*BOOTLOADER*



Default: grub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The documentation for the new options --composefs-native, --insecure, and --bootloader is incomplete. Please add descriptions for these options to explain their purpose and usage.

| **bootc usr-overlay** | Add a transient writable overlayfs on `/usr` |
| **bootc install** | Install the running container to a target |
| **bootc container** | Operations which can be executed as part of a container build |
| **bootc composefs-finalize-staged** | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description for the new subcommand bootc composefs-finalize-staged is missing. Please add a description to explain what this command does.

@cgwalters
Copy link
Collaborator Author

Split prep work for this to #1665

@cgwalters cgwalters force-pushed the composefs-by-default branch from 837dc44 to cccc2f8 Compare October 6, 2025 14:10
@cgwalters
Copy link
Collaborator Author

@jeckersb I ended up reworking this one to use the bootc-in-container for the sealing side because it would be too annoying in our CI to have it be on the host (which is ubuntu here).

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 6, 2025

I'm guessing this is something to do with me running just build-sealed inside of my toolbox, but I'm getting...

$ podman build -t localhost/bootc --build-arg=COMPOSEFS_FSVERITY=23e148ffffc8f37e2609bfe450bc945fa97e984a788d93fc8f93f19f7fc9ef9a9359c252969d0e12ac10caa7dd9a9427f9ad1c74696dae9be2b2575817386ab9 --build-arg=base=localhost/bootc-unsealed --secret=id=key,src=target/test-secureboot/db.key --secret=id=cert,src=target/test-secureboot/db.crt -f Dockerfile.cfsuki .
Error: failed to parse query parameter 'secrets': "[\"id=key,src=podman-build-secret3148315696\",\"id=cert,src=podman-build-secret3059655711\"]": rename /var/tmp/libpod_builder3803266185/build/podman-build-secret3148315696 /var/tmp/libpod_builder3803266185/podman-build-secret3148315696: no such file or directory
error: command exited with non-zero code `podman build -t localhost/bootc --build-arg=COMPOSEFS_FSVERITY=23e148ffffc8f37e2609bfe450bc945fa97e984a788d93fc8f93f19f7fc9ef9a9359c252969d0e12ac10caa7dd9a9427f9ad1c74696dae9be2b2575817386ab9 --build-arg=base=localhost/bootc-unsealed --secret=id=key,src=target/test-secureboot/db.key --secret=id=cert,src=target/test-secureboot/db.crt -f Dockerfile.cfsuki .`: 125
error: Recipe `build-sealed` failed on line 18 with exit code 1

@cgwalters
Copy link
Collaborator Author

Hmm I don't know what's up with that error. Offhand it's perhaps something to do with the source bind path when it's relative? Does it work if you make it an absolute path? What's your wrapper for podman in the toolbox look like?

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 7, 2025

Hmm I don't know what's up with that error. Offhand it's perhaps something to do with the source bind path when it's relative? Does it work if you make it an absolute path? What's your wrapper for podman in the toolbox look like?

Nope still doesn't work with absolute paths

⬢ [jeckersb@toolbx bootc]$ ls -l /var/home/jeckersb/git/bootc/target/test-secureboot/db.{crt,key}
-rw-r--r--. 1 jeckersb jeckersb 1854 Oct  6 15:54 /var/home/jeckersb/git/bootc/target/test-secureboot/db.crt
-rw-------. 1 jeckersb jeckersb 3272 Oct  6 15:54 /var/home/jeckersb/git/bootc/target/test-secureboot/db.key
⬢ [jeckersb@toolbx bootc]$ podman build -t localhost/bootc --build-arg=COMPOSEFS_FSVERITY=23e148ffffc8f37e2609bfe450bc945fa97e984a788d93fc8f93f19f7fc9ef9a9359c252969d0e12ac10caa7dd9a9427f9ad1c74696dae9be2b2575817386ab9 --build-arg=base=localhost/bootc-unsealed --secret=id=key,src=/var/home/jeckersb/git/bootc/target/test-secureboot/db.key --secret=id=cert,src=/var/home/jeckersb/git/bootc/target/test-secureboot/db.crt -f Dockerfile.cfsuki .
Error: failed to parse query parameter 'secrets': "[\"id=key,src=podman-build-secret2171653322\",\"id=cert,src=podman-build-secret2161445125\"]": rename /var/tmp/libpod_builder2211788443/build/podman-build-secret2171653322 /var/tmp/libpod_builder2211788443/podman-build-secret2171653322: no such file or directory

I don't have any wrapper for podman in my toolbox, but I conditionally set CONTAINER_CONNECTION to go over the podman socket. From .bashrc:

if [ -n "${TOOLBOX_PATH}" ]
then
    CONTAINER_CONNECTION=host
fi

and then...

$ podman system connection list
Name        URI                                       Identity    Default     ReadWrite
host        unix:///run/user/1000/podman/podman.sock              true        true
$ systemctl --user status podman.socket
● podman.socket - Podman API Socket
     Loaded: loaded (/usr/lib/systemd/user/podman.socket; enabled; preset: disabled)
     Active: active (listening) since Mon 2025-10-06 21:50:56 EDT; 13min ago
 Invocation: 0257bbab67374412a75a2c0b32b77018
   Triggers: ● podman.service
       Docs: man:podman-system-service(1)
     Listen: /run/user/1000/podman/podman.sock (Stream)
     CGroup: /user.slice/user-1000.slice/[email protected]/app.slice/podman.socket

I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow.

Given the eyeball test the changes look good to me. From CI it looks like it needs a cargo fmt and an update to test image pull check since now we're pulling the image which it expects to be absent?

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 7, 2025

I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow.

Testing a stripped-down case this morning, it works fine for me to pass secrets inside of the toolbox over podman-remote to the host. I've also verified that if I run the original podman command directly on the host, it works properly.

So I'm thinking now that it has something to do with passing secrets over podman-remote in combination with multistage builds.

@jeckersb
Copy link
Collaborator

jeckersb commented Oct 7, 2025

I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow.

Testing a stripped-down case this morning, it works fine for me to pass secrets inside of the toolbox over podman-remote to the host. I've also verified that if I run the original podman command directly on the host, it works properly.

So I'm thinking now that it has something to do with passing secrets over podman-remote in combination with multistage builds.

Ahhhhhh it's this - containers/podman#25314

I'll put in a separate PR to add that silly workaround to our .dockerignore

@cgwalters cgwalters force-pushed the composefs-by-default branch 5 times, most recently from 3dbc6dd to ee05675 Compare October 8, 2025 17:42
Signed-off-by: Colin Walters <[email protected]>
This ensures we're SHA-512 across the board.

Signed-off-by: Colin Walters <[email protected]>
- Change the install logic to detect UKIs and automatically
  enable composefs
- Move sealing bits to the toplevel
- Add Justfile entrypoints
- Add CI coverage that builds a disk image
- Change lints to ignore `/boot/EFI`

Signed-off-by: Colin Walters <[email protected]>
@cgwalters
Copy link
Collaborator Author

cgwalters commented Oct 8, 2025

OK it took me a bit too long to figure out the reason that we were getting cargo: command not found is that the default GHA Ubuntu runners seem to install Rust via rustup in the default runner user, so it isn't available as root.

This will get naturally fixed when we move to using bcvk here.

However...for now I moved the outer portion of the sealing back to shell script - we've already moved some of the inner portion into the Rust code.

@cgwalters cgwalters force-pushed the composefs-by-default branch from ee05675 to a58d0a3 Compare October 8, 2025 18:22
@cgwalters
Copy link
Collaborator Author

OK though, unfortunately right now tmt's virtual provisioner doesn't support uefi. That looks pretty easy to fix, but I'm a bit tempted to try out having bcvk libvirt replace the testcloud stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Updates to the documentation area/install Issues related to `bootc install`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants